-
Notifications
You must be signed in to change notification settings - Fork 3.3k
fix: Redirect spammy electron stderr to a debug sink #32188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
cypress
|
Project |
cypress
|
Branch Review |
fix/audio-error-stdout
|
Run status |
|
Run duration | 15m 48s |
Commit |
|
Committer | Cacie Prins |
View all properties for this run ↗︎ |
Test results | |
---|---|
|
1
|
|
0
|
|
11
|
|
0
|
|
630
|
View all changes introduced in this branch ↗︎ |
UI Coverage
0%
|
|
---|---|
|
4
|
|
0
|
Accessibility
100%
|
|
---|---|
|
0 critical
0 serious
0 moderate
0 minor
|
|
0
|
Tests for review
cypress/e2e/studio/studio.cy.ts • 1 failed test • app-e2e
Test | Artifacts | |
---|---|---|
Cypress Studio > writes the studio commands to the test block when the spec is updated on the file system and file watching is disabled |
Test Replay
Screenshots
|
"**/*.test.ts" | ||
], | ||
"compilerOptions": { | ||
"target": "ES2018", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we able to use a newer target?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could; this is what's in ts/tsconfig.json
though. I can't use that file directly because of "importsNotUsedAsValues"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't think we should be importing the config from @packages/ts
as it couples every package too tightly on how each package needs to compile. Maybe just me though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, absolutely. Just using the shared/common target that the rest of the repo does.
@@ -10,8 +10,12 @@ const paths = require('./paths') | |||
const install = require('./install') | |||
let fs = require('fs-extra') | |||
|
|||
const debugStderr = require('debug')('cypress:internal-stderr') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more of a nit but something like this?
const debugStderr = require('debug')('cypress:internal-stderr') | |
const debugStderr = require('debug')('cypress:electron:internal:stderr') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not limited to electron - if other third party libs write to stderr, they'll show up here too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah good point
packages/errors/tsconfig.json
Outdated
@@ -1,7 +1,7 @@ | |||
{ | |||
"extends": "../ts/tsconfig.json", | |||
"include": [ | |||
"src", | |||
"src", "../stderr-filtering/lib/logError.ts", "../stderr-filtering/lib/stderrSplitting", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird question but why do we need to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I... don't know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cacieprins Is this meant to be showing in these logs? https://app.circleci.com/pipelines/github/cypress-io/cypress/73649/workflows/d3326ad8-b967-489b-9143-f66b02582436/jobs/3070611
on DEV

@cacieprins Also in this output: https://app.circleci.com/pipelines/github/cypress-io/cypress/73649/workflows/d3326ad8-b967-489b-9143-f66b02582436/jobs/3070633 ![]() Seems to be missing these types of messages ![]() |
@jennifer-shehane The first issue seems related to the child Cypress in cy-in-cy tests not being piped through the filtering. The second is likely similar (either filtering being turned off w/ environment variable, or otherwise stderr output not being filtered by the pipeline set up in |
187d42a
to
52e67f2
Compare
Dismissing previous review - we'll make a followup issue for this.
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
Additional details
Rather than maintaining an ever-growing list of regular expressions to filter electron stderr output, I've opted to provide a holistic approach:
The errors lib now exports a few utility transform streams, and an error logging function. When the utility streams are used in conjunction with the tags that are exported alongside the error logging function, parent processes can effectively split "first party" stderr output from "third party" stderr output.
Steps to test
How has the user experience changed?
PR Tasks
cypress-documentation
?type definitions
?